-
-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
README Improvements #369
README Improvements #369
Conversation
- Added Note and generally clearer language in Getting Started for the purpose of directing users immediately to subtitle use if they wish - Added basic regex explanation, use instructions, and regex materials - Added tutorial video that covers installation, setup, and basic use of asbplayer when used as a Japanese vocabulary mining tool, including Yomitan and Anki. - Improved language in the note under Getting Started for better understanding of use-cases - Added year of publication to other community guide videos
README.md
Outdated
@@ -62,33 +62,36 @@ Thank you to all those who have translated asbplayer: | |||
If you are a non-English native, and would like to help translate asbplayer, please contact me on [Discord](https://discord.gg/ad7VAQru7m). | |||
|
|||
## Getting Started | |||
- *Note: asbplayer can either be used purely as a tool for subtitle control or as a deck mining tool (in which case you will need the subtitle functionality to mine). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line would be use case for the note syntax described here: https://github.com/orgs/community/discussions/16925
Also, I understand what you mean by "deck mining" but strictly speaking it should be "sentence mining" or "flashcard creation."
README.md
Outdated
@@ -62,33 +62,36 @@ Thank you to all those who have translated asbplayer: | |||
If you are a non-English native, and would like to help translate asbplayer, please contact me on [Discord](https://discord.gg/ad7VAQru7m). | |||
|
|||
## Getting Started | |||
- *Note: asbplayer can either be used purely as a tool for subtitle control or as a deck mining tool (in which case you will need the subtitle functionality to mine). | |||
If you want to strictly learn how to use asbplayer's subtitle features and are not interested in sentence or vocabulary mining, you can read about them under the [Detailed Usage](#detailed-usage) header.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true that the information about the subtitle features can be found under "Detailed usage" but the reader still has to read and hunt for it since a lot of the documentation is still about making flashcards. So I would suggest these changes:
- Be more specific, for example: "just follow step 6."
- Make the "subtitle features" below "Detailed usage" more obvious such as by grouping them together or adding more information. Currently the only features explicitly documented are subtitle detection, offset, and regex filtering.
README.md
Outdated
@@ -212,6 +215,22 @@ Response: | |||
} | |||
``` | |||
|
|||
### Filtering Subtitles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The phrase "Filtering subtitles" is a bit ambiguous IMO, as it could mean "filtering individual subtitles" or "filtering subtitle text."
README.md
Outdated
@@ -212,6 +215,22 @@ Response: | |||
} | |||
``` | |||
|
|||
### Filtering Subtitles | |||
|
|||
If you'd like to filter subtitles, one way to do so is by using regexing. asbplayer will match any sequence following regex pattern and remove the matches. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above about "filtering subtitles."
Also I would not consider "regexing" to be a commonly-used expression, even for more technical people.
asbplayer will match any sequence following regex pattern and remove the matches.
- "a regex pattern" or "a specified regex pattern"
- asbplayer does not necessarily "remove the matches" even though that is a common use-case. Strictly speaking asbplayer replaces matched text with specified replacement text, which you have detailed below. So I at the very least I would say "can match ... and remove" instead of "will match ... and remove."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, got it, making these changes now.
Should I mention that there is a difference between English () and Japanese()?
something like
Note: Japanese parentheses are usually different characters than the ones found on english keyboards: "()" versus "()"
Or should I leave that to the user to discover since this is already kind of a niche thing?
README.md
Outdated
- Remove names enclosed by parenthesis to indicate speakers (i.e. "**(山田)** 元気ですか?") | ||
- Remove indications enclosed by brackets that sound or music that is playing (i.e. "**\[PLAYFUL MUSIC]**") | ||
|
||
Helpful materials on regexing: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as about "regexing." I think it's clearer/more helpful to say "writing regular expressions" or similar.
I got feedback from one of my peers on this PR and they suggested either labeling one of the guide videos as latest or tagging each guide with a version, rather than a year to imply recency (because a year is a long time). I like the latter but it might be a bit of trouble finding out the version of asbplayer at the time each was published.
What do you think? |
- changed note under getting started to have official formatting - separated community guides, removed year tags, added "(latest)" tag to the most recently made video - reorganized detailed usage for better flow, both from the getting started note that links to it and within itself - subtitle setup and controls grouped together - anki setup and controls grouped together - other basic features group together (side panel, keyboard shortcuts, audio track selection) - advanced features grouped together (one click mining, websocket) - added note about japanese parentheses under subtitle filtering section - subtitle filtering clarifications - added instructions about downloading subtitles feature
changed "other features" to "other basic features" so that it makes more sense on why it comes before "advanced features"
README.md
Outdated
@@ -62,33 +62,40 @@ Thank you to all those who have translated asbplayer: | |||
If you are a non-English native, and would like to help translate asbplayer, please contact me on [Discord](https://discord.gg/ad7VAQru7m). | |||
|
|||
## Getting Started | |||
> [!NOTE] | |||
> asbplayer can either be used purely as a tool for subtitle control or as a flashcard creation tool (in which case you will need the subtitle functionality to mine vocabulary or sentences). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(in which case you will need the subtitle functionality to mine vocabulary or sentences)
Nitpick: I don't think this blurb is necessary to understand the note.
README.md
Outdated
- [Refold's installation guide](https://www.youtube.com/watch?v=Pv4Sp01Uh64) | ||
- [Refold's sentence mining guide (Japanese + European languages)](https://www.youtube.com/watch?v=jXO4gmCmcNE) | ||
- [Sentence Mining: Learning Japanese From Anime (Japanese)](https://www.youtube.com/watch?v=B60cj69MSmA) | ||
- [How to Setup and Use ASBPlayer for Vocab Mining (Japanese) (Latest)](https://www.youtube.com/watch?v=D1tlb7zo8Og&ab_channel=pooks_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the value in providing multiple guides is mostly in the varying content and presentation, I don't think it's particularly valuable to annotate a single video as being the "latest" one. For example, people studying European languages will ignore any video that's marked Japanese, regardless of whether it's marked "Latest" or not.
README.md
Outdated
@@ -97,9 +104,16 @@ Otherwise, the following steps will work for any language: | |||
|
|||
## Detailed usage | |||
|
|||
### Enhancing streaming video with asbplayer features | |||
First, install the Chrome [extension](https://chromewebstore.google.com/detail/asbplayer-language-learni/hkledmpjpaehamkiehglnbelcpdflcab). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this necessarily makes sense directly under Detailed usage
since there are plenty of users who don't install the extension to use asbplayer with local files.
README.md
Outdated
### Enhancing streaming video with asbplayer features | ||
First, install the Chrome [extension](https://chromewebstore.google.com/detail/asbplayer-language-learni/hkledmpjpaehamkiehglnbelcpdflcab). | ||
|
||
In order to make use of any of asbplayer's features (those outside of subtitles, e.g. mining), subtitles must first be loaded into the extension, with the context of loading them into streaming video or with local video files. The following section instructs how to do this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't actually accurate, since asbplayer can be used without installing the extension. This would be more accurate:
In order to make use of any of asbplayer's features, subtitles (or an empty subtitle track) must first be loaded, either onto a streaming video or a local video file. The following section instructs how to do this.
Even though using more words might be more precise in meaning, in general I think it's clearer to use less, more simple words since it's easier on the reader's eyes. Many people reading this documentation are not English natives so we should try to make it easy for them to read as well.
README.md
Outdated
|
||
Install the Chrome [extension](https://chromewebstore.google.com/detail/asbplayer-language-learni/hkledmpjpaehamkiehglnbelcpdflcab). There are a number of ways to enhance streaming video with asbplayer: | ||
### Injecting and detecting subtitles into streaming video |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sections under Subtitle setup and features
I would use smaller headings to indicate that each subsection belongs to this larger section. So use ####
instead of ###
. Similar for other subsections.
To be consistent with the section below about local files I would say:
Enhancing streaming video with asbplayer-controlled subtitles
README.md
Outdated
@@ -108,29 +122,10 @@ Install the Chrome [extension](https://chromewebstore.google.com/detail/asbplaye | |||
|
|||
asbplayer features will then be accessible for that video. | |||
|
|||
### Enhancing local video files with asbplayer features | |||
### Enhancing local video files with asbplayer controlled subtitles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: "asbplayer-controlled" instead of "asbplayer controlled"
README.md
Outdated
- [RexEgg regex cheatsheet](https://www.rexegg.com/regex-quickstart.html) | ||
- Test regex with the [Regexr webtool](https://regexr.com/) | ||
|
||
> Note: Japanese parentheses are usually different characters than the ones found on english keyboards: "()" versus "()" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this note about parantheses can be removed. Many people reading this don't care about Japanese and I'd prefer asbplayer to be seen as language-agnostic.
README.md
Outdated
|
||
### Downloading subtitle files | ||
|
||
Once subtitles have been loaded into asbplayer using one of the previously mentioned methods, you can choose to download them either by opening the side panel and clicking the `Download Subtitles as SRT` button in the top right, or by accessing the [asbplayer website](https://killergerbah.github.io/asbplayer/) and clicking the same button in the top left. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not totally accurate, the website will have that button only if subtitles have been loaded into it somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for going back-and-forth with me on this. Since it's very important for the README to be as useful and clear as possible I am being very opinionated in this review. I hope you understand.
It's no problem, I totally understand the value in accuracy, good flow, and clear language when it comes to this sort of thing. |
- generally clarified language - removed note about parenthesis - removed blurb - removed "(latest)" tag - changed "first, install the extension" to "to use asbplayer, you can choose..." to avoid confusion about usage - changed subsections from ### to ####
README.md
Outdated
@@ -62,33 +62,39 @@ Thank you to all those who have translated asbplayer: | |||
If you are a non-English native, and would like to help translate asbplayer, please contact me on [Discord](https://discord.gg/ad7VAQru7m). | |||
|
|||
## Getting Started | |||
> [!NOTE] | |||
> asbplayer can either be used purely as a tool for subtitle control or as a flashcard creation tool. If you want to strictly learn how to use asbplayer's subtitle features and are not interested in sentence or vocabulary mining, you can read about them under the [Subtitle setup and features](#subtitle-setup-and-features) header. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can make this shorter. e.g:
"asbplayer is both a subtitle control and flashcard creation tool. If are not interested in flashcards, and only want to use asbplayer's subtitle features, you can read about them under Subtitle setup and features."
Side note: it's easier to understand what "them" means when "asbplayer's subtitle features" is closer to it.
README.md
Outdated
4. [Configure](https://killergerbah.github.io/asbplayer/?view=settings) asbplayer to create cards via AnkiConnect using your deck and note type. | ||
|
||
5. Enhance a video using asbplayer and subtitle files. | ||
- *Note: In order to use asbplayer's flashcard audio and screenshot capabilities, you will need to set the deck up accordingly. You can learn how to do this from one of the [community guides](#community-guides).* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be shorter:
"Use one of the community guides to learn how to set up an Anki deck."
We are already telling the user to setup an Anki deck, so I don't think we need to repeat that instruction.
README.md
Outdated
@@ -97,9 +103,16 @@ Otherwise, the following steps will work for any language: | |||
|
|||
## Detailed usage | |||
|
|||
### Enhancing streaming video with asbplayer features | |||
To use asbplayer, you can either choose to install the Chrome [extension](https://chromewebstore.google.com/detail/asbplayer-language-learni/hkledmpjpaehamkiehglnbelcpdflcab) or use the [webtool](https://killergerbah.github.io/asbplayer/). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the more common word "website" and not "webtool."
I think we can also explain when to use the extension or when to use the website e.g:
"To use asbplayer with streaming video, install the Chrome extension. Otherwise, use the website."
README.md
Outdated
|
||
#### Downloading subtitle files | ||
|
||
Once loaded into the extension, you can choose to download subtitles by opening the side panel and clicking the `Download Subtitles as SRT` button in the top right. The same can be done with subtitles loaded into the webtool by accessing the [asbplayer website](https://killergerbah.github.io/asbplayer/) and clicking the same download button in the top left. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion to shorten and use "website" instead of "webtool":
"Once loaded into the extension, you can download the subtitles by opening the side panel and clicking the Download Subtitles as SRT
button in the top-right. You can also download subtitles via the website by clicking the same download button in the top-left."
I would also remove the link to the website. Linking the website here may be a bit misleading since subtitles can't be downloaded simply by just accessing the website.
README.md
Outdated
|
||
Once loaded into the extension, you can choose to download subtitles by opening the side panel and clicking the `Download Subtitles as SRT` button in the top right. The same can be done with subtitles loaded into the webtool by accessing the [asbplayer website](https://killergerbah.github.io/asbplayer/) and clicking the same download button in the top left. | ||
|
||
> Note: if you have used the [regex feature](#filtering-subtitle-text), it will alter the loaded subtitles and as a result alter the .srt that is downloaded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be shorter:
Note: The regex feature will alter the
.srt
that is downloaded.
README.md
Outdated
Under the MISC section in asbplayer settings, locate the "Subtitle regex filter" textbox. Enter an appropriate regex to filter desired content. | ||
You can replace filtered content similarly by entering a string into the "Subtitle regex filter text replacement" textbox. Leaving this blank will simply remove the content. | ||
|
||
Potential use cases for filtering include: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shorten:
"Useful regular expressions:"
And specify the regex you're referring to in each bullet point.
README.md
Outdated
|
||
Install the Chrome [extension](https://chromewebstore.google.com/detail/asbplayer-language-learni/hkledmpjpaehamkiehglnbelcpdflcab). There are a number of ways to enhance streaming video with asbplayer: | ||
--- | ||
### Subtitle setup and features |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shorten: "Subtitle features"
README.md
Outdated
- Remove indications enclosed by brackets that sound or music that is playing (i.e. "**\[PLAYFUL MUSIC]**") | ||
|
||
Helpful materials on writing regular expressions: | ||
- [GeeksForGeeks regex tutorial](https://www.geeksforgeeks.org/write-regular-expressions/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's limit the resources we provide to a single high-quality one that doesn't have ads. Most people who know nothing about regexes will likely not want to click through multiple links to just learn about regexes. I suggest this one: https://regexlearn.com/playground
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a wonderful resource!!
made requested changes, also added a ► character before each #### title because I thought these titles were too similar in size to regular text. We can change it to ➤ if we want it a bit bigger, but I think I like the smaller better, the extra whitespace looks a bit more breathable. LMK what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making all of the changes I requested. One more final opinionated nitpick: can you remove the triangles next to each subheading? I think it's easier to read without them. After that I'll merge the PR. Thanks again.
removed ►
Thank you!! |
Yay!! So happy it got merged, thank you!! |
Added Note and generally clearer language in Getting Started for the purpose of directing users immediately to subtitle use if they wish
Added basic regex explanation, use instructions, and regex materials
Added tutorial video that covers installation, setup, and basic use of asbplayer when used as a Japanese vocabulary mining tool, including Yomitan and Anki.
Improved language in the note under Getting Started for better understanding of use-cases
Added year of publication to other community guide videos